-
-
Notifications
You must be signed in to change notification settings - Fork 150
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Automatic Generic-Derived Qualifiers #145
Automatic Generic-Derived Qualifiers #145
Conversation
@dtolnay I'm looking for two pieces of advice on this PR:
With the latter, I'll be able to finish implementation of the generics handling for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- the format parser is in impl/src/fmt.rs.
- I think traversing type parameters of fields of type Type::Path should be sufficient. I don't think the complexity of making this more exhaustive is warranted.
@dtolnay: Thanks on the fmt reference- I must have missed that. It looks like I'll need to restructure it to get at the information I'm looking for, however. As for not going deeper than first-layer Path types- that doesn't cover enum FromGenericError<Indirect> {
#[error("Tadah")]
SourceEmbedded(#[from] DirectEmbedding<Indirect>),
} Isolating the impl<Indirect> Error for FromGenericError<Indirect>
where
DirectEmbedding<Indirect>: Error,
Indirect: 'static, // <--- This line
Self: Debug + Display; As you can see from my latest push, I am looking at using |
@dtolnay I attempted to use syn's The tests cover the cases you described in this comment, and they are all now passing. |
I realized that this implementation does not yet support With the amount of additional code necessary to add it, it may be worth doing as a separate PR. Similarly, we may want to set up tasks for generic support in One thing I've noticed, @dtolnay, is significant redundancy between both the Do you have any ideas on how we can refactor |
I just spent a few hours attempting to make As I suspected in my prior comment, I think support for these other modes ( |
…derivation Replaced `#[error(transparent)]` usages, as they are not yet supported in automatic generic constraint derivation. Removed generic bounds from `TunnelRegistrationError` and `TunnelNamingError` as they are conventionally incorrect and made unnecessary with the new bound-generation capability. See `thiserror` PR dtolnay/thiserror#145
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up implementing this in an easier and less brittle way in #148, so I'll go ahead and close this implementation. Thanks anyway for the PR!
@@ -94,6 +101,122 @@ impl Display<'_> { | |||
self.args = args; | |||
self.has_bonus_display = has_bonus_display; | |||
} | |||
|
|||
pub fn iter_fmt_types(&'a self, fields: &'a [Field]) -> Vec<DisplayFormatMarking<'a>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's problematic that this 172 lines of changes in fmt.rs is duplicating the entire format string parser in a way that is completely detached from (and incompatible with) the existing format string processing.
Take a look at my version which sticks a comparatively tiny piece of code (24 lines) in the existing format string codepath to wire up implied bounds as a natural part of processing the format string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, using a member on the Display
type was a better move. I like that it also allows bounds for the other display types. I think the one thing I'd change given infinite time would be splitting Display<'_>
into pre-expanded and post-expanded types, and keeping the template and non-template sections around as an ADT, then resolving the field rename hacks on demand.
}; | ||
|
||
// Strip expanded identifier-prefixes since we just want the non-shorthand version | ||
let name = if name.starts_with("field_") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a red flag – this code is trying to reverse engineer (lossily) information that already exists where the format string was already processed inside expand_shorthand
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed- that's a consequence of invoking it at the late-phase location I did- I think I probably should've augmented it by changing expand_shorthand
to some sort of generic type that maps it out, but the problem was that my generic intersection code required knowledge of the generics before it could determine what was a generic.
The majority of my time ended up sunk into writing parse methods using syn::Parser
, only to find out that it couldn't handle imbalanced bracketing at any point, and didn't provide lower-level access than tokens. Having not spent much time with iterative parsers like the one in expand_shorthand
, I couldn't see a straightforward way to extract the full information I needed from it without re-plumbing its state outputs.
Alas- as you showed- that was the right move ^^;
.display | ||
.iter() | ||
.flat_map(|d| d.iter_fmt_types(input.fields.as_slice())) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general these changes in expand.rs are way more code than I would expect to need to add for this feature. Please see #148 for the approach I used instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your Field.contains_generic
approach combined with the scope
abstraction seem to be the biggest contributors to code reduction, and InferredBounds
is doing some serious work considering its length.
I'm curious, however, if there's some way to describe the "intent" for each bound as it's read, allowing a single InferredBounds
-esque type to track the various usage forms to generalize the solution beyond error
and display
bounds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that is how https://github.com/dtolnay/reflect works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll keep an eye out for opportunities to use that library- but I suspect it would be a bit much in the way of overhead for this task? Then again, being a proc macro, its dependencies (hopefully?) aren't compiled into the binary it helps to produce unless explicitly requested.
self.generics.iter().filter(|(_k, v)| **v).map(|(k, _v)| k) | ||
} | ||
|
||
#[allow(dead_code)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No dead code please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad- that one got left behind- I must've forgotten to tag it todo
and wasn't clear I'd use it or not before the feature was ready.
syn::visit::visit_type_path(self, i); | ||
} | ||
|
||
fn visit_type_param(&mut self, i: &'ast syn::TypeParam) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not reachable — except maybe in contrived code like field: [T; { fn f<T>() {}; 0 }]
where you actually don't want the behavior here of considering the inner T
TypeParam as being a use of the outer T
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used this to deep-search for generic usages because- when a generic wasn't used- I was running into circumstances where fixed types would succeed in compilation where they should not, because it generated impossible/unlikely bounds, such as PathBuf: Display
, which you seem to have resolved using the PathAsDisplay
trait in the past. My solution was to deep-scan for generics in types, and only add bounds for fields which used at least one generic.
|
||
member_refs | ||
.iter() | ||
.map(|(_m, f, is_display)| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_m
shouldn't be getting put into member_refs
if nothing is going to use it.
pub fn mark_from<'a, TMarkedSource: IntoIterator<Item = &'a proc_macro2::Ident> + 'a>( | ||
&'a mut self, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'a
lifetime requirements in this signature are never used.
- pub fn mark_from<'a, TMarkedSource: IntoIterator<Item = &'a proc_macro2::Ident> + 'a>(
- &'a mut self,
+ pub fn mark_from<'a, TMarkedSource: IntoIterator<Item = &'a proc_macro2::Ident>>(
+ &mut self,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum. I thought if I had one explicit (the one used in IntoIterator::Item
) they were supposed to all be explicit. Neat.
@dtolnay Awesome- your new version includes the sort of refactoring I was struggling to specify! I believe you may be able to test an "upper bound" for these requirements such that superfluous ones cannot be introduced by use of a mechanism along the lines of the following "compile to prove" functions, by supplying expected-minimum-bounded generics as a maximum upper bound: #[allow(unused)]
fn static_display_upper_bounds<TDisplayOnly, TDebugOnly, TNoFormat>(
instance: &EnumCompound<TDisplayOnly, TDebugOnly, TNoFormat>,
f: &mut std::fmt::Formatter<'_>,
) -> std::fmt::Result
where
TDisplayOnly: std::fmt::Display,
TDebugOnly: std::fmt::Debug,
{
Display::fmt(&instance, f)
}
#[allow(unused)]
fn static_transparent_upper_bounds<T>(
instance: &StructTransparentGeneric<T>,
f: &mut std::fmt::Formatter<'_>,
) -> std::fmt::Result
where
T: std::error::Error,
{
std::error::Error::source(&instance);
Display::fmt(&instance, f)
} |
Fixes #79 compliant with feedback from #143.
This PR attempts to produce bounds for generics only where necessitated by their usages.